Hello @D-Veda, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

The latest CDC-ECM implementation has improved the following issues

1. Reduce the use of mutexes, hold locks only in necessary competitive environments, and use atomic flags in USB callbacks to avoid data contention
2. Refine and streamline the descriptor matching mechanism
3. Simplify the MAC address query mechanism by first querying according to the default LANGID, and then obtain the LANGID in turn if it fails
4. Improve the two-way concurrency mechanism, interrupt in multiplexes xfer and associated buf, bulk IN multiplexes xfer, only when data needs to be forwarded to the Ethernet protocol stack to assign new buf associated with xfer, bulk out callback multiplexes xfer to trigger ZLP transmission
5. All Ethernet interface operations implement error rollback operations (e.g., set packet filter / multicast filter fails, rolling back the previous value)
6. Streamline the xfer recycling mechanism, only when the dequeue triggers the callback, the xfer will be released, and the rest of the cases are just paused xfer, canceling the dlist tracking mechanism
7. Streamlined implementation of multicast filters/statistics, updated CDC-ECM statistics-related macros

Just to mention, not every optional feature needs to be in the initial PR, for example statistics are not needed for networking to work. it is a nice to have and can be added in a later PR. If you really want it in here, put it in its own commit.

Just to mention, not every optional feature needs to be in the initial PR, for example statistics are not needed for networking to work. it is a nice to have and can be added in a later PR. If you really want it in here, put it in its own commit.

The statistics part feature is moved out now.

Sorry for this very incomplete review as I am short on time today.

#99097 (comment) does not seem to be interested in modifying his PR to make it closer to this one. Would you be interested in trying to rebase your PR on top of his content?

If finding modifications needed to #99097, they could then be submitted as review comments to his PR.

@josuah I'm sorry for replying to you so late.

Compared to #99097, this PR implements more complete functionality required for CDC-ECM:

• Packet / Multicast Filter
• Complete and accurate descriptor resolution
• Precise MAC string lookup
• Complete instance lifecycle management
• Complete link status notification
• statistics is removed from this PR

Originally, this PR was based on #99097. However, during the design and testing process, it was found that in order to fully support the above functions, I needed to make a lot of modifications to #99097 to ensure that the basic functions of CDC-ECM can have a complete instance lifecycle and that no abnormal situations will occur (including ensuring consistency of data flow between network subsystem <-> usb host subsystem, and avoiding resource management or data contention issues such as Use-After-Free). And from reviewing perspective, I need to combine the commits together and add Santhosh Charles as co-author. So the result is that #99097 's commits is merged with my changes and Santhosh Charles is added as co-author.

If #99097 is adopted, I would need to perform extensive refactoring on top of it. From a practical standpoint, I believe directly adopting this PR is the most appropriate approach.

This PR also has advantages over #99097 in basic data stream processing:

• Use zero-copy to directly forward the USB buffer to the network subsystem in the bulk in direction.
• Linearization of the buffer was uniformly applied in the bulk out direction.
• All incoming traffic uses pre-allocated xfer and buffer, and reuses data as much as possible in the callback (the entire xfer / buf process has achieved the lowest possible latency in data forwarding).
• Use k_sem to manage the maximum number of concurrent send requests in the bulk out direction.
• Use a separate buffer pool to manage forwarded data

Based on the above, I believe we should directly adopt this PR. Adding changes similar to this PR to #99097 would only increase the effort required, which is practically a waste of time and energy.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Hello,

Thanks for this PR, been using it successfully with some device.

However, there's interesting bug sometimes happening (been going on for a while but with newest commits there was also some error message with it) that sometimes the usbh-cdc-ecm driver is in some kind of a lock state but it gets opened up once i run "net ping -I 2 <ip_addr>" where interface 2 is the usbh-cdc-ecm iface.

[00:04:38.519,201] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
uart:~$ net ping -I 2 25.25.25.25
PING 25.25.25.25
28 bytes from 25.25.25.25 to 192.168.50.2: icmp_seq=1 ttl=52 time=173 ms
28 bytes from 25.25.25.25 to 192.168.50.2: icmp_seq=2 ttl=52 time=175 ms
28 bytes from 25.25.25.25 to 192.168.50.2: icmp_seq=3 ttl=52 time=171 ms

After this the pipe opens up and data starts to once again flow from it.

Here's some code that claude produces to "fix" it; i could not test this myself properly since I can rarely reproduce this bug.

static int bulk_in_req_cb(struct usb_device *const udev, struct uhc_transfer *const xfer)
{
	struct cdc_ecm_host_data *host_data = xfer->priv;
	struct net_buf *buf = xfer->buf;
	struct net_buf *new_buf = NULL;
	uint16_t max_segment_size;
	int err;

	if (xfer->err == -ECONNRESET) {
		LOG_INF("The bulk IN transfer is cancelled");
		net_buf_unref(buf);
		usbh_xfer_free(udev, xfer);
		return 0;
	}

	if (xfer->err != 0) {
		LOG_WRN("The bulk IN transfer failed: %d", xfer->err);
	}

	if (!atomic_test_bit(&host_data->flags, CDC_ECM_DEVICE_FLAG_CONNECTED) ||
	    !atomic_test_bit(&host_data->flags, CDC_ECM_DEVICE_FLAG_FORWARDING)) {
		return 0;
	}

	max_segment_size = CDC_ECM_DESC_MAX_SEGMENT_SIZE(&host_data->desc);

	/* Pre-allocate replacement buffer BEFORE handing the current one to the stack.
	 * If allocation fails, we drop this packet and reuse the existing buffer,
	 * guaranteeing the RX slot stays alive.
	 */
	if (xfer->err == 0 && buf->len > 0 && buf->len <= max_segment_size) {
		new_buf = net_buf_alloc(&usbh_cdc_ecm_pool, K_NO_WAIT);
	}

	if (new_buf != NULL) {
		/* Hand off old buf to network stack, install new_buf */
		err = forward_received_pkt(host_data, buf);
		if (err != 0) {
			/* Stack rejected it; buf is back in our hands via the
			 * cleanup inside forward_received_pkt. Drop new_buf and
			 * reuse the original.
			 */
			net_buf_unref(new_buf);
			net_buf_reset(buf);
		} else {
			xfer->buf = NULL;
			err = usbh_xfer_buf_add(udev, xfer, new_buf);
			if (err != 0) {
				LOG_ERR("Failed to add buffer to bulk IN transfer: %d", err);
				net_buf_unref(new_buf);
				return 0;
			}
		}
	} else {
		/* Either the transfer errored, the length was bad, or the pool
		 * was exhausted. Drop the packet and reuse the buffer.
		 */
		net_buf_reset(buf);
	}

	err = usbh_xfer_enqueue(udev, xfer);
	if (err != 0) {
		LOG_ERR("Failed to continue bulk IN transfer: %d", err);
	}

	return 0;
}

Some explanation of the fix and what is the potential bug:

Issue: The bulk IN callback orphans transfers when net_buf_alloc fails after handing the current buffer to the stack — xfer->buf is set to NULL, then on alloc failure we goto done without re-enqueuing. Each occurrence permanently kills one RX slot; after RX_PIPELINE_DEPTH hits, the RX pipe is dead until replug. This explains the intermittent "stuck pipe" reports where outbound traffic (e.g. net ping) appears to "wake up" the interface — it doesn't actually recover the dead slots, it just coincides with pool pressure easing.
Fix: Pre-allocate the replacement buffer before handing the current one off. If allocation fails (or the frame is invalid — too short, too long, or transfer errored), drop the packet, reset and reuse the existing buffer, and always re-enqueue. The RX slot can no longer be lost; worst case under memory pressure is a dropped packet, which the network stack handles fine.
Also added a length floor (>= sizeof(struct net_eth_hdr)) to avoid forwarding runt frames to the stack.

Also:

Side note (not blocking this PR): even with the fix, I still see periodic bulk_in_req_cb: The bulk IN transfer failed: -5 warnings at ~5s intervals during idle. The pipe stays alive and throughput is fine, so these appear to be benign — likely the device emitting some periodic frame the HC reports as -EIO

[00:04:03.885,157] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:08.885,157] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:13.885,143] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:21.785,152] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:26.785,145] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:31.785,151] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:43.085,152] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:51.285,149] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:04:56.285,143] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:01.285,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:25.785,151] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:34.785,150] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:39.785,148] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:44.785,142] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:53.885,150] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:05:58.885,141] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:06:03.885,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:06:08.885,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:06:28.785,151] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:29.985,144] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:34.985,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:39.985,141] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:44.985,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:49.985,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:07:58.685,139] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:03.685,146] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:08.685,151] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:13.685,148] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:18.685,142] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:23.685,139] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5
[00:08:28.685,147] <wrn> usbh_cdc_ecm: bulk_in_req_cb: The bulk IN transfer failed: -5

Thanks.

It seems like this code did not however manage to fix that, and I am frankly not sure is it even related to this driver or Zephyr RTOS network stack. :) But look into it and if doesn't make any sense or you don't think it is in this driver, then ignore all the previous comments since I am quite unsure what is the problem. :) All i know is that running "net ping -I 2 <ip_addr>" fixes it when sometimes the usbh-cdc-ecm or something is stuck. :) Cheers.